Skip to content

feat(unitree): forward aes_128_key to WebRTC driver for G1 firmware >=1.5.1#2117

Open
mihai-chiorean wants to merge 9 commits into
dimensionalOS:mainfrom
mihai-chiorean:feat/forward-aes-128-key
Open

feat(unitree): forward aes_128_key to WebRTC driver for G1 firmware >=1.5.1#2117
mihai-chiorean wants to merge 9 commits into
dimensionalOS:mainfrom
mihai-chiorean:feat/forward-aes-128-key

Conversation

@mihai-chiorean
Copy link
Copy Markdown

Motivation

Unitree G1 firmware 1.5.1 introduced a data2=3 WebRTC handshake that requires a per-device AES-128 key. Without it the connection fails inside unitree_webrtc_connect with RSA key format is not supported (pycryptodome trying to parse ciphertext bytes as an RSA key).

The per-device key is fetched from Unitree's cloud once via unitree-fetch-aes-key --email YOU --sn <serial> and then cached locally. The upstream driver legion1581/unitree_webrtc_connect@v2.1.1 accepts it through an aes_128_key= kwarg on UnitreeWebRTCConnection. The dimos wrapper at dimos/robot/unitree/connection.py never forwarded it, so anyone on G1 firmware >=1.5.1 cannot use the dimos Unitree integration today.

Change

UnitreeWebRTCConnection.__init__ now accepts an optional aes_128_key: str | None = None kwarg and falls back to the UNITREE_AES_128_KEY environment variable. The value is forwarded to the underlying LegionConnection only when non-empty, so the call is byte-identical to the previous behaviour when unset.

def __init__(self, ip: str, mode: str = "ai", aes_128_key: str | None = None) -> None:
    ...
    if aes_128_key is None:
        aes_128_key = os.environ.get("UNITREE_AES_128_KEY")
    extra: dict[str, Any] = {"aes_128_key": aes_128_key} if aes_128_key else {}
    self.conn = LegionConnection(WebRTCConnectionMethod.LocalSTA, ip=self.ip, **extra)

Verification

Tested on a Unitree G1 EDU+ (model string G1_Edu+) at firmware 1.5.1.1. With UNITREE_AES_128_KEY exported in the shell, dimos run unitree-g1-basic proceeds past the WebRTC handshake on 192.168.123.161:9991 and the robot accepts motion-switcher / wireless-controller commands as before. Without the env var, the same command reproduces RSA key format is not supported and the handshake never completes — same behaviour as main.

No breaking change

The kwarg is optional and defaults to None. The env-var lookup is opt-in (only consulted when the kwarg is None). Callers on G1 firmware <1.5.1 and all Go2 robots see no behavioural change — the **extra is empty and the LegionConnection call is byte-identical to the old one.

Optional follow-up (not in this PR)

pyproject.toml currently pins unitree-webrtc-connect-leshy>=2.0.7. The aes_128_key kwarg + the _decrypt_data1_v3 path that consumes it landed in legion1581/unitree_webrtc_connect@v2.1.1. If unitree-webrtc-connect-leshy is a downstream rebuild of that repo and is already on >=2.1.1, no change is needed and this PR is enough on its own. If it's still tracking 2.0.x, the dep will also need to be bumped (e.g. via [tool.uv.sources] pointing at git+https://github.com/legion1581/unitree_webrtc_connect.git@v2.1.1). Happy to follow up in a separate PR if maintainers confirm the situation.

…=1.5.1

Unitree G1 firmware 1.5.1 added a data2=3 WebRTC handshake that requires
a per-device AES-128 key (fetched from Unitree cloud once via
`unitree-fetch-aes-key --email YOU --sn <serial>`). The upstream
`legion1581/unitree_webrtc_connect@v2.1.1` accepts this via the
`aes_128_key=` kwarg; without it the handshake fails with
`RSA key format is not supported` from pycryptodome reading ciphertext
bytes.

`UnitreeWebRTCConnection.__init__` now accepts an optional
`aes_128_key` kwarg and falls back to the `UNITREE_AES_128_KEY`
environment variable so existing call sites do not need to change.
When the value is unset the call to `LegionConnection` is byte-identical
to the previous behaviour, so this is a no-op for G1 firmware <1.5.1
and for Go2.

Verified against a Unitree G1 EDU+ on firmware 1.5.1.1 via
`dimos run unitree-g1-basic` - the WebRTC handshake on
192.168.123.161:9991 succeeds with UNITREE_AES_128_KEY exported and
reproduces the `RSA key format is not supported` failure without it.
@leshy
Copy link
Copy Markdown
Contributor

leshy commented May 17, 2026

reason for legion client fork is that unitree-webrtc-connect-leshy had a more efficinet lidar data parser and we had some issues with aiortc package pinning and had to fork it as well

legion1581/unitree_webrtc_connect@master...leshy:unitree_webrtc_connect:master
I assume lidar stuff is merged in the legion lib and all is resolved now

for FDEs (or anyone else here in the thread:) - we need to validate this on current g1s/go2s (ensure my lidar changes are now merged to legion client, transition to legion lib, test on our robots, make sure installation works, merge this)

self.stop_timer: threading.Timer | None = None
self.cmd_vel_timeout = 0.2
self.conn = LegionConnection(WebRTCConnectionMethod.LocalSTA, ip=self.ip)
# Per-device AES-128 key required by G1 firmware >= 1.5.1 (data2=3 WebRTC handshake).
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also include GO2 firmware 1.1.15 as it has the same issue and this should fix it for the GO2 too !

@codecov
Copy link
Copy Markdown

codecov Bot commented May 17, 2026

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
dimos/robot/unitree/connection.py 33.33% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@natcl
Copy link
Copy Markdown

natcl commented May 17, 2026

had a more efficinet lidar data parser

I did notice a degradation in navigation performance on our go2 edu when I switched to the upstream library, might be worth checking if the changes are there.

Address PR review feedback from dimensionalOS#2117:

1. G1Config and G1HighLevelWebRtcConfig now expose 'aes_128_key' as an
   optional declarative config field, forwarded to UnitreeWebRTCConnection.
   Users on G1 firmware >= 1.5.1 can now set it via blueprint config
   without resorting to the UNITREE_AES_128_KEY env var. The env-var
   fallback in UnitreeWebRTCConnection.__init__ is preserved as-is.

2. Adds dimos/robot/unitree/test_connection.py with five test cases that
   cover the kwarg-forwarding logic without hardware:
   - default behaviour: no kwarg, no env → aes_128_key not forwarded
     (byte-identical to pre-PR call)
   - explicit kwarg is forwarded verbatim
   - env-var fallback when kwarg is None
   - explicit kwarg beats env-var (precedence)
   - empty-string kwarg is treated as 'no key' (truthiness guard)

Tests are pure-Python: LegionConnection is mocked, .connect() is patched
to a no-op. Default pytest selector (-m 'not tool ...') will run them.
Comment thread dimos/robot/unitree/connection.py Outdated
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@mihai-chiorean
Copy link
Copy Markdown
Author

I Was at a hackathon, but I'll get this sorted out shortly.

@spomichter
Copy link
Copy Markdown
Contributor

Thanks! @ruthwikdasyam or @Krishna can assist with any testing here

Mirrors the G1 wiring from this PR so Go2 firmware >= 1.1.15 (same data2=3
WebRTC handshake change) can pass the per-device AES-128 key. Adds the
field to Go2's ConnectionConfig, accepts it as a kwarg on make_connection,
and forwards it to UnitreeWebRTCConnection in the webrtc branch. Other
branches (replay, mujoco, dimsim) are unaffected.

Also fixes the test docstring greptile flagged after the 'if not
aes_128_key' guard landed: the test asserting empty-string + unset env →
no forwarding had a misleading docstring claiming the kwarg was 'treated
as no key'. Rewrites the docstring and adds a sibling test covering the
case that actually motivated the guard fix: empty-string kwarg + set env
→ env value wins (the YAML/JSON 'aes_128_key: ""' regression case).

Addresses natcl's review comment on PR dimensionalOS#2117.
@mihai-chiorean
Copy link
Copy Markdown
Author

Force-pushed 6bff60d44 addressing the open review items:

  • @natcl — Go2 firmware ≥1.1.15 coverage added. aes_128_key now flows through go2/connection.py (ConnectionConfig field → make_connection(..., aes_128_key=...)UnitreeWebRTCConnection) mirroring the G1 pattern, plus through go2/fleet_connection.py so fleet followers get the same forwarding (with an inline comment noting per-device-key heterogeneous fleets are a future per-IP-mapping follow-up).
  • @greptile-apps — The previous if aes_128_key is Noneif not aes_128_key truthiness fix landed in fceccd7c0. Updated the now-misleading docstring on test_empty_string_kwarg_* and added a sibling test_empty_string_kwarg_uses_env_when_set covering the YAML/JSON aes_128_key: "" regression you flagged.
  • Added a targeted unit test go2/test_connection.py::test_make_connection_webrtc_forwards_aes_128_key to pin the new make_connection ↔ UnitreeWebRTCConnection contract.

Ran the changes through codex review --commit HEAD after each iteration. Codex caught the missed Go2FleetConnection.start() forwarding (now fixed) and signed off on the amended commit.

@leshy — re. the upstream legion lib transition + lidar perf, leaving that for a separate PR (also flagged on this PR thread by @natcl who noticed degraded nav on Go2 EDU after the upstream switch). Happy to scope that follow-up once you confirm the lidar parser changes have landed in legion upstream.

@dimensionalOS dimensionalOS deleted a comment from greptile-apps Bot May 19, 2026
@dimensionalOS dimensionalOS deleted a comment from greptile-apps Bot May 19, 2026
@leshy
Copy link
Copy Markdown
Contributor

leshy commented May 19, 2026

I think ideally should bump to latest unitree_webrtc_connect for this to be usable.

my lidar changes have landed upstream looks like (legion1581/unitree_webrtc_connect#46) but let's still compare perforamnce between https://github.com/leshy/unitree_webrtc_connect (what we use now) and upgraded latest official unitree_webrtc_connect

because of

I did notice a degradation in navigation performance on our go2 edu when I switched to the upstream library, might be worth checking if the changes are there.

@ruthwikdasyam has experience with lidar data profiling so can look into this. if performance is the same, we can commit a bump in pyproject to official lib and merge all together

@mihai-chiorean
Copy link
Copy Markdown
Author

I think ideally should bump to latest unitree_webrtc_connect for this to be usable.

my lidar changes have landed upstream looks like (legion1581/unitree_webrtc_connect#46) but let's still compare perforamnce between https://github.com/leshy/unitree_webrtc_connect (what we use now) and upgraded latest official unitree_webrtc_connect

because of

I did notice a degradation in navigation performance on our go2 edu when I switched to the upstream library, might be worth checking if the changes are there.

@ruthwikdasyam has experience with lidar data profiling so can look into this. if performance is the same, we can commit a bump in pyproject to official lib and merge all together

want me to do that here or is that for a separate PR...? happy to update it, a little worried about the blast radius of updating a package like that since I'm new to the repo.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 19, 2026

Greptile Summary

This PR fixes WebRTC connectivity for Unitree G1 robots on firmware ≥1.5.1 by threading an optional aes_128_key parameter through the connection stack — from config objects and make_connection down to UnitreeWebRTCConnection, which forwards it to the upstream LegionConnection driver only when the value is truthy.

  • UnitreeWebRTCConnection.__init__ now accepts aes_128_key: str | None = None and falls back to the UNITREE_AES_128_KEY environment variable using a truthiness guard (if not aes_128_key) that correctly treats both None and "" as absent.
  • G1Config, G1HighLevelWebRtcConfig, ConnectionConfig (Go2), and FleetConnectionConfig (via inheritance) all gain the new optional field; follower robots in a fleet share the primary's key, with a code comment acknowledging the per-device limitation for heterogeneous fleets.
  • New unit tests in test_connection.py and go2/test_connection.py cover the no-key, explicit-kwarg, env-var-fallback, precedence, and empty-string edge cases.

Confidence Score: 5/5

Safe to merge — the change is purely additive, all new parameters are optional with None defaults, and the call to LegionConnection is byte-identical to the old one when no key is supplied.

The truthiness guard (if not aes_128_key) correctly handles both None and empty-string inputs, the env-var fallback is opt-in only when no kwarg is present, and the **extra dict is empty when no key is available — leaving all existing callers and firmware versions unaffected. The new tests cover every edge case including the empty-string scenario flagged in the prior review thread.

No files require special attention.

Important Files Changed

Filename Overview
dimos/robot/unitree/connection.py Core change: adds optional aes_128_key kwarg with if not truthiness guard (handles both None and "") and env-var fallback; forwarded to LegionConnection only when truthy — byte-identical to pre-PR behaviour when unset.
dimos/robot/unitree/g1/connection.py Adds `aes_128_key: str
dimos/robot/unitree/g1/effectors/high_level/webrtc.py Adds aes_128_key to G1HighLevelWebRtcConfig and forwards it to UnitreeWebRTCConnection; wiring is correct and consistent with G1 connection module.
dimos/robot/unitree/go2/connection.py Adds aes_128_key to ConnectionConfig and make_connection signature; forwarded to UnitreeWebRTCConnection in the webrtc branch only — replay/mujoco/dimsim paths unaffected.
dimos/robot/unitree/go2/fleet_connection.py Forwards aes_128_key to follower robots via make_connection; inherits the field from ConnectionConfig through FleetConnectionConfig. Known limitation (per-device key for heterogeneous fleets) is documented in a code comment.
dimos/robot/unitree/test_connection.py Comprehensive unit tests for UnitreeWebRTCConnection: covers no-key, explicit kwarg, env-var fallback, kwarg-over-env precedence, and the empty-string edge cases that were flagged in a previous review.
dimos/robot/unitree/go2/test_connection.py Targeted test for the Go2 routing path; pins that make_connection forwards aes_128_key as a kwarg to UnitreeWebRTCConnection using a monkeypatched stub.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Caller passes aes_128_key to config] --> B[G1Connection / GO2Connection reads config field]
    B --> C[UnitreeWebRTCConnection.__init__]
    C --> D{kwarg truthy?}
    D -- Yes --> F[Use kwarg value]
    D -- No --> E[Look up env var]
    E --> G{env var set?}
    G -- Yes --> F
    G -- No --> H[key remains None]
    F --> I[extra dict with aes_128_key]
    H --> J[extra = empty dict]
    I --> K[LegionConnection with aes_128_key]
    J --> L[LegionConnection unchanged call]
Loading

Reviews (9): Last reviewed commit: "Merge branch 'main' into feat/forward-ae..." | Re-trigger Greptile

@mihai-chiorean
Copy link
Copy Markdown
Author

I think ideally should bump to latest unitree_webrtc_connect for this to be usable.
my lidar changes have landed upstream looks like (legion1581/unitree_webrtc_connect#46) but let's still compare perforamnce between https://github.com/leshy/unitree_webrtc_connect (what we use now) and upgraded latest official unitree_webrtc_connect
because of

I did notice a degradation in navigation performance on our go2 edu when I switched to the upstream library, might be worth checking if the changes are there.

@ruthwikdasyam has experience with lidar data profiling so can look into this. if performance is the same, we can commit a bump in pyproject to official lib and merge all together

want me to do that here or is that for a separate PR...? happy to update it, a little worried about the blast radius of updating a package like that since I'm new to the repo.

@leshy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants